Finish GraphCache implementation for Pub/Sub #66

Yadunund · 2023-11-15T18:11:56Z

This PR:

Refactors how we store and update the GraphCache within each session. Specifically we move away from a YAML::Node to an unordered_map (although implementing a conversion from our custom map to YAML may be helpful for logging or transmitting the graph as a serialized YAML over the network).
Makes the switch to rely on liveliness tokens to broadcast changes to the ROS graph. The previous implementation of relying on put/del/get convey the changes is retained but commented out as it could be reused to support discovery with zenoh-pico.
Implements some basic topic/node introspection functions in rmw (the rest will be done in a separate PR).

Note: Without the router, nodes will not be able to discover each other since multicast discovery is disabled by default in the node's session config. Instead, nodes will receive discovery information about other peers via the router's gossip function.

francocipollone

Great Job @Yadunund 🚀
I left a bunch of not-game-changing comments, ptal

rmw_zenoh_cpp/src/detail/graph_cache.cpp

francocipollone · 2023-11-16T13:20:00Z

rmw_zenoh_cpp/src/detail/graph_cache.cpp

+        auto erase_it = found_node->pubs.begin();
+        for (; erase_it != found_node->pubs.end(); ++erase_it) {
+          const auto & pub = *erase_it;
+          if (pub.topic == node->pubs.at(0).topic &&
+            pub.type == node->pubs.at(0).type &&
+            pub.qos == node->pubs.at(0).qos)
+          {
+            break;
+          }
+        }


nit: use find_if here and there

Suggested change

auto erase_it = found_node->pubs.begin();

for (; erase_it != found_node->pubs.end(); ++erase_it) {

const auto & pub = *erase_it;

if (pub.topic == node->pubs.at(0).topic &&

pub.type == node->pubs.at(0).type &&

pub.qos == node->pubs.at(0).qos)

{

break;

}

}

auto erase_it = std::find_if(found_node->pubs.begin(), found_node->pubs.end(), [&node](const auto &pub) {

return pub.topic == node->pubs.at(0).topic &&

pub.type == node->pubs.at(0).type &&

pub.qos == node->pubs.at(0).qos;

});

Then proceed as you do:

if (erase_it != found_node->pubs.end()) { // Found a matching element, erase it found_node->pubs.erase(erase_it); //... //... //... }

rmw_zenoh_cpp/src/detail/graph_cache.cpp

rmw_zenoh_cpp/src/rmw_get_topic_names_and_types.cpp

rmw_zenoh_cpp/src/detail/graph_cache.cpp

Signed-off-by: Yadunund <[email protected]>

* Switch to liveliness tokens Signed-off-by: Yadunund <[email protected]> * Use zc APIs instead of macros to resolve liveliness api issues Signed-off-by: Yadunund <[email protected]> --------- Signed-off-by: Yadunund <[email protected]>

Signed-off-by: Yadunund <[email protected]>

When playing around with this I found that the types weren't being properly filled in, and also weren't being demangled. Fix both of those things here, as well as do a lot of cleanup. Signed-off-by: Chris Lalancette <[email protected]>

clalancette · 2023-11-17T20:32:22Z

I ended up rebasing this on the latest, as well as fixing some bugs that I found. I also addressed most of @francocipollone 's comments, with the exception of a switch to find_if. I'll take a closer look at that later.

However, with all of that done, ros2 node list and ros2 topic list -t both work entirely correctly for me. There is still an open TODO in there where a topic with multiple types on it won't be shown properly. To fix that, we'll need to revamp the way some of the data is put together. That's a project for next week.

francocipollone · 2023-11-17T21:12:30Z

I ended up rebasing this on the latest, as well as fixing some bugs that I found. I also addressed most of @francocipollone 's comments, with the exception of a switch to find_if. I'll take a closer look at that later.

However, with all of that done, ros2 node list and ros2 topic list -t both work entirely correctly for me. There is still an open TODO in there where a topic with multiple types on it won't be shown properly. To fix that, we'll need to revamp the way some of the data is put together. That's a project for next week.

Confirmed, it is working correctly. It seems it was my bad: I had some issues with the daemon. Basically, it was already running (I was working on another branch) and then when I checkouted this branch and built I hadn't stopped the daemon.

Yadunund · 2023-11-18T01:06:27Z

Thanks for cleaning this up Chris! I'll work on massaging the cache data structures to support multiple types on the same topic in a separate PR to keep the diffs minimal. If both of you are okay with the code here, should we merge this in first?

Signed-off-by: Yadunund <[email protected]>

clalancette · 2023-11-18T12:47:40Z

Thanks for cleaning this up Chris! I'll work on massaging the cache data structures to support multiple types on the same topic in a separate PR to keep the diffs minimal. If both of you are okay with the code here, should we merge this in first?

Yeah, I think we should do that. The open issues here are 1) to switch to find_if as suggested by Franco, and 2) fix multiple types per topic. As long as we fix both of those in a follow-up, I'm good with merging this.

This reverts commit fe77eef.

* Add storage plugin config to router config Signed-off-by: Yadunund <[email protected]> * Fix non empty node namespace Signed-off-by: Yadunund <[email protected]> * Rely on unordered_map instead of yaml for graph cache Signed-off-by: Yadunund <[email protected]> * Update graph cache with publisher data Signed-off-by: Yadunund <[email protected]> * Implement basic publisher introspection Signed-off-by: Yadunund <[email protected]> * Switch to liveliness tokens (#67) * Switch to liveliness tokens Signed-off-by: Yadunund <[email protected]> * Use zc APIs instead of macros to resolve liveliness api issues Signed-off-by: Yadunund <[email protected]> --------- Signed-off-by: Yadunund <[email protected]> * Cleanup yaml-cpp Signed-off-by: Yadunund <[email protected]> * Cleanup previous graph cache impl Signed-off-by: Yadunund <[email protected]> * Refactor topic cache Signed-off-by: Yadunund <[email protected]> * Implement liveliness tokens for subscriptions and update graph Signed-off-by: Yadunund <[email protected]> * Fix types and type mangling. When playing around with this I found that the types weren't being properly filled in, and also weren't being demangled. Fix both of those things here, as well as do a lot of cleanup. Signed-off-by: Chris Lalancette <[email protected]> * Update README Signed-off-by: Yadunund <[email protected]> --------- Signed-off-by: Yadunund <[email protected]> Signed-off-by: Chris Lalancette <[email protected]> Co-authored-by: Chris Lalancette <[email protected]>

Yadunund marked this pull request as ready for review November 16, 2023 11:35

francocipollone reviewed Nov 16, 2023

View reviewed changes

Yadunund and others added 11 commits November 17, 2023 14:54

Add storage plugin config to router config

995b2de

Signed-off-by: Yadunund <[email protected]>

Fix non empty node namespace

5199c1d

Signed-off-by: Yadunund <[email protected]>

Rely on unordered_map instead of yaml for graph cache

59c28d1

Signed-off-by: Yadunund <[email protected]>

Update graph cache with publisher data

69d2ead

Signed-off-by: Yadunund <[email protected]>

Implement basic publisher introspection

5be7199

Signed-off-by: Yadunund <[email protected]>

Switch to liveliness tokens (#67)

aa831ef

* Switch to liveliness tokens Signed-off-by: Yadunund <[email protected]> * Use zc APIs instead of macros to resolve liveliness api issues Signed-off-by: Yadunund <[email protected]> --------- Signed-off-by: Yadunund <[email protected]>

Cleanup yaml-cpp

a56eec2

Signed-off-by: Yadunund <[email protected]>

Cleanup previous graph cache impl

c518431

Signed-off-by: Yadunund <[email protected]>

Refactor topic cache

559819c

Signed-off-by: Yadunund <[email protected]>

Implement liveliness tokens for subscriptions and update graph

428cace

Signed-off-by: Yadunund <[email protected]>

Fix types and type mangling.

f7711c7

When playing around with this I found that the types weren't being properly filled in, and also weren't being demangled. Fix both of those things here, as well as do a lot of cleanup. Signed-off-by: Chris Lalancette <[email protected]>

clalancette force-pushed the yadu/graph-cache branch from a0f47b1 to f7711c7 Compare November 17, 2023 20:30

Update README

2d8e446

Signed-off-by: Yadunund <[email protected]>

clalancette approved these changes Nov 18, 2023

View reviewed changes

Yadunund merged commit fe77eef into rolling Nov 19, 2023
5 checks passed

delete-merged-branch bot deleted the yadu/graph-cache branch November 19, 2023 15:55

Yadunund added a commit that referenced this pull request Nov 19, 2023

Revert "Finish GraphCache implementation for Pub/Sub (#66)"

c84e70d

This reverts commit fe77eef.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Finish GraphCache implementation for Pub/Sub #66

Finish GraphCache implementation for Pub/Sub #66

Yadunund commented Nov 15, 2023 •

edited

Loading

francocipollone left a comment

francocipollone Nov 16, 2023

clalancette commented Nov 17, 2023

francocipollone commented Nov 17, 2023

Yadunund commented Nov 18, 2023

clalancette commented Nov 18, 2023

Finish GraphCache implementation for Pub/Sub #66

Finish GraphCache implementation for Pub/Sub #66

Conversation

Yadunund commented Nov 15, 2023 • edited Loading

francocipollone left a comment

Choose a reason for hiding this comment

francocipollone Nov 16, 2023

Choose a reason for hiding this comment

clalancette commented Nov 17, 2023

francocipollone commented Nov 17, 2023

Yadunund commented Nov 18, 2023

clalancette commented Nov 18, 2023

Yadunund commented Nov 15, 2023 •

edited

Loading